Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Equativ Bid Adapter: initial release, Smartadserver Bid Adapter: take lowest floor to send to endpoint #12326

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

krzysztofequativ
Copy link
Contributor

@krzysztofequativ krzysztofequativ commented Oct 14, 2024

Type of change

  • New bidder adapter
  • Feature

@patmmccann patmmccann self-assigned this Oct 19, 2024
@krzysztofequativ krzysztofequativ changed the title Equativ Bid Adapter: initial release Equativ Bid Adapter: initial release, Smartadserver Bid Adapter: Take lowest floor to send to endpoint Oct 21, 2024
@krzysztofequativ krzysztofequativ changed the title Equativ Bid Adapter: initial release, Smartadserver Bid Adapter: Take lowest floor to send to endpoint Equativ Bid Adapter: initial release, Smartadserver Bid Adapter: take lowest floor to send to endpoint Oct 21, 2024
@krzysztofequativ
Copy link
Contributor Author

@patmmccann, could you share when the review could be perform, please? We have a pilot publisher for the new adapter and would like to schedule the work. Thanks in advance for your feedback.

@patmmccann patmmccann self-requested a review October 24, 2024 12:50
@patmmccann
Copy link
Collaborator

Review feedback on slack

delete imp.dt;

imp.bidfloor = imp.bidfloor || spec.getMinFloor(bidRequest);
imp.secure = Number(window.location.protocol === 'https:');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Window doesn't matter, the safeframe is always secure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. However, I see many other adapters which calculate the secure property in the same way. Does it mean that they also should be updated?

* @param bidRequest
* @returns {number}
*/
getMinFloor: (bidRequest) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You guys have this same code block in two adapters, please import it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

});

return isPlainObject(floor) && !isNaN(floor.floor) ? floor.floor : DEFAULT_FLOOR;
return floors.length ? Math.min(...floors) : DEFAULT_FLOOR;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 357 below is throwing a warning:

/home/circleci/Prebid.js/modules/smartadserverBidAdapter.js
357:1 warning The type 'syncs' is undefined jsdoc/no-undefined-types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@krzysztofequativ
Copy link
Contributor Author

Thanks @patmmccann for your immediate response. I've fixed all the mentioned points. Please continue reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants